Derive TypeInfo for own types by dvdplm · Pull Request #72 · paritytech/scale-info

dvdplm · 2021-03-03T15:36:52Z

Builds upon/supersedes #21

Derive TypeInfo for all of scale-infos own types.

Derive TypeInfo for all of scale-infos types

Robbepop · 2021-03-03T16:10:01Z

derive/src/lib.rs

-            :: #scale_info ::meta_type::<#ty_ident>()
+            #scale_info::meta_type::<#ty_ident>()


as before for all non-scale-info crates this should still prepend :: to signal root crate path.

It doesn't compile with the :: and I don't know why tbh, but I think that when deriving for scale-info itself, and we have extern crate self as _scale_info;, then ::_scale_info::path::to::something doesn't work. :/

The extern crate self as _scale_info; is not required since Rust edition 2018 and I don't think we should support Rust edition 2015 tbh because it actually resolves many problems we are facing otherwise such as this one.

So either this crate should produce: crate:: paths or whatever crate alias a dependency uses for scale-info prepended with ::. To be honest I am not sure how counterintuitive it is to use the derive macros for the crate that exposes them. I see a gain but issues like these let me feel it might be better to simply provide manual implementations for the few types in the re-exporting crate. Happy to learn a better approaches for this problem. In ink! we also use manual implementations for our derive macros in the root crates.

To be more concrete: While the proc-macro-crate crate is actually useful in these cases it makes use of file I/O while expanding a proc. macro. There are discussions about encapsulating proc. macros in stricter environments that would break proc. macros such as these. Generally tooling such as rust-analyzer has good reasons why proc. macros that are "impure" should not be relied on.

To be honest I am not sure how counterintuitive it is to use the derive macros for the crate that exposes them.

How do you mean "counterintuitive"? I take it you mean it's forcing things a bit?

I think it's a good litmus test for the crate, especially since the type zoo in scale-info is fairly advanced and gives us a "free test suite" of sorts. The amount of manual implementations we'd have to maintain is pretty large so if we can derive most of them that's a win imo.

I am concerned by the increased complexity this brings. I will see what I can do to make things a wee bit less messy.

I am concerned by the increased complexity this brings. I will see what I can do to make things a wee bit less messy.

I think this commit is makes things much cleaner.

Proc. macros bring overhead with them. So some people prefer using crates but not their provided proc. macros which is why we find derive crate feature on some crates to opt-out. It is just nice to be able to separate concerns between the crate that provides definitions and the crate that provides the proc. macro implementations. As soon as the crate providing definitions makes use of the proc. macro you no longer have the separation and a direct dependency instead. I personally am not against this but overhead can be serious in some cases.
The biggest problem is that derive macros that can be used today in their parent crate require this type of "hack" that is encapsulated by the proc-macro-crate crate currently. Using this "hack" in any proc. macro turns the proc. macro impure. For example it can no longer enjoy acceleration by the techniques behind the watt crate which some people believe to be the future compilation model for all proc. macros since it is super fast and more secure than today's proc. macros.

Don't get me wrong: I see the tradeoffs and see some of the pros of doing this. But I still consider this feature to be taking in some amount of technical depth into the project in order to have the anticipated feature implementation for which we will probably have to pay back in the future. Technical depth is death to all progress so I always am careful how much of it I accumulate in projects.

The use of proc-macro-crate predates this PR so the technical debt is already there;

This PR does not introduce a mandatory dependence on scale-info-derive: it's opt-in through a feature (as it should be, couldn't agree with you more);

I think we should have a conversation about the use of proc-macro-crate; you make a really good point about it making the crate "impure" and we should hash out the trade-offs involved. But I'm not sure this PR is the right place for that?

derive/src/lib.rs

Prepend `::` to paths when not deriving for `scale-info`. Alias self as `scale-info` Prefer unwrap_or_else to expect Add todo about broken features (break trybuild tests)

Move scale-info crate name construction to own function

dvdplm · 2021-03-04T09:26:15Z

@Robbepop @ascjones Do you have any thoughtsabout what to do about

scale-info/src/meta_type.rs

Lines 54 to 67 in 5a165b1

    
           // TODO: this is a total hack. Not sure what we can do here. 
        
           impl TypeInfo for MetaType { 
        
               type Identity = Self; 
        
               fn type_info() -> Type<MetaForm> { 
        
                   Type::builder() 
        
                       .path(Path::new("MetaType", "meta_type")) 
        
                       .composite( 
        
                           Fields::named() 
        
                               // .field_of::<fn() -> Type<MetaForm>>("fn_type_info", "fn() -> Type<MetaForm>") 
        
                               .field_of::<core::num::NonZeroU64>("type_id", "TypeId") 
        
                       ) 
        
               } 
        
           }

?

Robbepop · 2021-03-04T09:56:37Z

@Robbepop @ascjones Do you have any thoughtsabout what to do about

scale-info/src/meta_type.rs

Lines 54 to 67 in 5a165b1

// TODO: this is a total hack. Not sure what we can do here.

impl TypeInfo for MetaType {

type Identity = Self;

fn type_info() -> Type<MetaForm> {

Type::builder()

.path(Path::new("MetaType", "meta_type"))

.composite(

Fields::named()

// .field_of::<fn() -> Type<MetaForm>>("fn_type_info", "fn() -> Type<MetaForm>")

.field_of::<core::num::NonZeroU64>("type_id", "TypeId")

)

}

}

?

Hmmm it is a tricky one. Do we actually need to derive for it? It is only really used as generic parameter in a raw function signature type so maybe PhantomData kind is enough?

ascjones · 2021-03-04T10:00:37Z

MetaType is never SCALE encoded so shouldn't need TypeInfo at all. We only need the PortableForm

test_suite/Cargo.toml

Robbepop · 2021-06-22T09:33:04Z

This seems to be stable. Is there a good reason to keep this open and eventually merge it or should we not pursue it further?

dvdplm · 2021-06-22T10:23:38Z

This seems to be stable. Is there a good reason to keep this open and eventually merge it or should we not pursue it further?

I think we should merge it. It's not super useful in itself, but does provide a sanity check. Happy to update it; won't make a fuss if @ascjones feel it's not needed any longer. :)

ascjones · 2021-06-22T10:51:22Z

It's absolutely something we will need, it just wasn't on the critical path for substrate integration so I have neglected looking at it.

Happy to give this a review once it is updated with master and we can get it in in time for the next release.

Robbepop · 2021-06-22T12:26:36Z

We'd need to make extra sure that before/after semantics are equal with respect to the newly derived traits.

Cargo.toml

derive/src/lib.rs

ascjones · 2021-06-28T15:33:41Z

Use case for this would be generating equivalent types in another language e.g. https://github.com/polkadot-js/api/blob/58cc197c890881db5a604a4ad98c2446a16e8631/packages/types/src/interfaces/scaleInfo/types.ts.

Robbepop · 2021-06-29T14:16:16Z

.github/workflows/rust.yml

    - name: check-features
      run: |
        cargo check --no-default-features --features bit-vec
+        cargo check --no-default-features --features dogfood


Do we want to keep this optional crate feature? What purpose does it have?

That is a good question. I fail to see the reason to keep this as a feature. @ascjones you ok making it non-optional?

Just saw Giles's PR with this stuff in and fwiw I think it doesn't need to be behind a feature flag.

gilescope · 2022-07-16T12:50:29Z

derive/src/lib.rs

+//         Err(e) => return Err(syn::Error::new(Span::call_site(), e)),
+//     };
+//     Ok(crate_ident)
+


Wasn't entirely sure which version of this function to use so went with master. If that's fine we can delete this commented out version.

gilescope · 2022-07-16T12:51:18Z

Have just brought this up to date as realised I might need this to decode metadata in the browser in rust.

* support for frame-metadata to have TypeInfo

Don't use a feature in the derive crate

84b3bb4

Derive TypeInfo for all of scale-infos types

ascjones mentioned this pull request Mar 3, 2021

Derive TypeInfo impls for meta type definitions #21

Closed

Robbepop reviewed Mar 3, 2021

View reviewed changes

dvdplm added 6 commits March 3, 2021 19:00

Review grumbles

c2f0f06

Prepend `::` to paths when not deriving for `scale-info`. Alias self as `scale-info` Prefer unwrap_or_else to expect Add todo about broken features (break trybuild tests)

Less noise is better: build a TokenStream2 containing the "::" prefix

b4ba154

Move scale-info crate name construction to own function

Use crate:: when deriving for scale-info itself

b1d26c2

Derive TypeInfo for UntrackedSymbol

22fb440

wip

00aae34

Add ref to ticket that blocks deriving Typeinfo for Type

5a165b1

Remove TypeInfo impl for MetaType

9ac6306

dvdplm self-assigned this Mar 4, 2021

Clippy is right

39f8459

dvdplm marked this pull request as ready for review March 4, 2021 10:13

dvdplm requested a review from ascjones March 4, 2021 10:13

Cleanup

7db6778

dvdplm commented Mar 4, 2021

View reviewed changes

test_suite/Cargo.toml Outdated Show resolved Hide resolved

dvdplm added 2 commits March 4, 2021 13:49

Merge branch 'master' into dp-doogfood

687c757

Merge branch 'master' into dp-doogfood

7f4fae7

dvdplm added 4 commits June 25, 2021 16:57

Merge branch 'master' into dp-doogfood

1d8ff8c

Remove NonZero* impls

5c22f19

Cleanup

58dc3d2

Don't bind <… as HasCompact>::Type

e73c061

dvdplm requested a review from Robbepop June 25, 2021 15:08

ascjones reviewed Jun 28, 2021

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

derive/src/lib.rs Outdated Show resolved Hide resolved

derive/src/lib.rs Outdated Show resolved Hide resolved

dvdplm added 2 commits June 28, 2021 17:07

Merge remote-tracking branch 'origin/master' into dp-doogfood

03bf034

Review grumbles

c7bfb52

dvdplm added 4 commits June 29, 2021 07:41

Fix comment

89d4499

Merge branch 'master' into dp-doogfood

14055e1

Address review feedback

c7c19fc

fmt

d3259c5

Robbepop reviewed Jun 29, 2021

View reviewed changes

dvdplm added 2 commits August 24, 2021 14:09

Merge branch 'master' into dp-doogfood

54deaf4

cleanup

5fd7525

ascjones mentioned this pull request Nov 15, 2021

Generating and Publishing Equivalent TypeScript Types paritytech/frame-metadata#32

Open

Merge branch 'master' into dp-doogfood

2d7530d

gilescope reviewed Jul 16, 2022

View reviewed changes

gilescope added 2 commits July 16, 2022 15:22

Fixup after merge

ba2e94a

additional support to allow frame-metadata to derive TypeInfo (#163)

3420903

* support for frame-metadata to have TypeInfo

		:: #scale_info ::meta_type::<#ty_ident>()
		#scale_info::meta_type::<#ty_ident>()

Conversation

dvdplm commented Mar 3, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Robbepop Mar 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Robbepop Mar 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dvdplm commented Mar 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Robbepop commented Mar 4, 2021

Uh oh!

ascjones commented Mar 4, 2021

Uh oh!

Uh oh!

Robbepop commented Jun 22, 2021

Uh oh!

dvdplm commented Jun 22, 2021

Uh oh!

ascjones commented Jun 22, 2021

Uh oh!

Robbepop commented Jun 22, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ascjones commented Jun 28, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gilescope commented Jul 16, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Robbepop Mar 3, 2021 •

edited

Loading

Robbepop Mar 4, 2021 •

edited

Loading

dvdplm commented Mar 4, 2021 •

edited

Loading